✨ Adding feature for allocating requested IP #997

ksahil12 · 2025-04-10T08:26:46Z

What this PR does / why we need it:
Requirement: Allocate a specific to IPAddressClaim or IPClaim when created with special label.
Solution: During creation of IPAddressClaim, user can provide a special label i.e. ipAddress: <IP_TO_BE_REQUESTED>. Once IPAddressClaim goes for allocation logic in ip_pool_manager, we can handle the allocation of this IP based on the requestedIP and available IPs.
For a happy path scenario, when allocation is successful, we will create IPAddress with the same IP and update IPPool index with the allocated IP.
For a scenario where user has requested an IP which does not belong to range or not available, we can patch IPAddressClaim CR with the error mentioning the same.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #969

metal3-io-bot · 2025-04-10T08:26:58Z

Hi @ksahil12. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tuminoid · 2025-04-10T09:21:09Z

/ok-to-test
/cc @kashifest @peppi-lotta @adilGhaffarDev

peppi-lotta · 2025-04-11T06:52:28Z

I tested a situation where pools preAllocations are:

Pre Allocations:
  test1:  192.168.0.12
  test2:  192.168.0.15
  test3:  192.168.0.17

and I the applied an IPClaim:

apiVersion: ipam.metal3.io/v1alpha1
kind: IPClaim
metadata:
  name: test2
  namespace: default
  labels:
    ipAddress: 192.168.0.20
spec:
  pool:
    name: pool1
    namespace: default

IpClaim then has error Error Message: Requested IP not available, which is not true. Could you add something like "PreAllocation and requested ip address are conflicting" or something along those lines.

Otherwise the feature seems to be working as I expected.

peppi-lotta · 2025-04-11T06:52:31Z

Could you also add tests for situation
One pool, with start and existing address, ipAddress label present and requested ipAddress in preAllocations with conflicting ipclaim name

One pool, with start and existing address, ipAddress label present and requested ipAddress in preAllocations with non-conflicting ipclaim name

It is important to understand how the new feature works with preAllocations as those are curretly an important part of CAPM3 upgrading.

ksahil12 · 2025-04-11T08:50:24Z

Thanks for suggestion @peppi-lotta.
Updated the scenario. Also added test cases.

peppi-lotta · 2025-04-11T12:07:55Z

Please rebase your commits into one :)

ksahil12 · 2025-04-12T11:12:24Z

@peppi-lotta updated the PR.

peppi-lotta · 2025-04-14T06:27:18Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

peppi-lotta · 2025-04-15T06:33:43Z

/lgtm
This seems to work well and as expected. Good job and thank you for contributing!

ksahil12 · 2025-04-15T10:15:04Z

@peppi-lotta thanks for the review.
@tuminoid can you help with approvals as it's mentioned above to assign PR to you.

lentzi90

Sorry for noticing so late, but could we change this to use annotations instead of labels?
Labels are in general meant as identifying attributes, not for modifying controller behavior. Annotations on the other hand, are good for this and what is commonly used (e.g. in ingress controllers).

ksahil12 · 2025-04-17T07:17:45Z

@lentzi90 agreed. We generally use annotations for providing additional/supportive data. @peppi-lotta Please share your thoughts as well. I can work on changing it once finalized.

peppi-lotta · 2025-04-17T08:24:49Z

Annotation seems more appropriate for this.

lentzi90

Great! Now it looks good to me, just two typos below
/approve
We need to think carefully if we want this in the next release or hold it until after. We already had the beta release so introducing new features at this stage is a bit risky. If it isn't urgent to get this in, I would say it is better to wait for the release-1.10 branch to be created until we merge.
I'm putting hold so we don't accidentally merge it.
/hold

ipam/ippool_manager_test.go

metal3-io-bot · 2025-04-17T09:52:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lentzi90]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: khandelwalsahil <[email protected]>

ksahil12 · 2025-04-17T09:59:23Z

update the PR. please approve again.
@lentzi90 actually we were looking forward for this change to get merged. Please check if it's possible to merge it to next release (1.10).
By the way, you must be working on a release branch for 1.10 release, right? In that case, is there any harm in merging this to main?

lentzi90 · 2025-04-17T10:07:16Z

We haven't created the release branch yet. As soon as we do, there is no issue merging this to main.
Would it be enough for you to get this on main for now (and then obviously included in v1.11)?

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

ksahil12 · 2025-04-17T10:12:08Z

I will definitely strive for getting it in 1.10 release :) and would request the same. Please let me know feasibility of it.
Generally, how long are the release cycles (duration between two)?

lentzi90 · 2025-04-17T10:17:05Z

/cc @kashifest @adilGhaffarDev
what do you think? Can we take this in v1.10? I don't see it as a huge risk, but it is a bit late in the cycle.
We should probably think about formalizing code freeze deadlines for future cycles...

lentzi90 · 2025-04-17T10:28:13Z

Generally, how long are the release cycles (duration between two)?

We follow Cluster API on this. It is roughly 4 months between minor releases.

adilGhaffarDev · 2025-04-17T11:03:43Z

what do you think? Can we take this in v1.10? I don't see it as a huge risk, but it is a bit late in the cycle.

Could we add some documentation for this feature, explaining what it does and how users can make use of it?
I am fine with merging this once it is documented.

lentzi90 · 2025-04-17T11:35:10Z

Good point! I think the perfect place for this documentation would be here: https://book.metal3.io/ipam/introduction#ipclaim, which is coming from here

ksahil12 · 2025-04-17T12:40:09Z

sharing doc update PR: metal3-io/metal3-docs#515
please review.
cc: @lentzi90 @adilGhaffarDev @kashifest @peppi-lotta @tuminoid

ksahil12 · 2025-04-22T08:21:08Z

@tuminoid @lentzi90 can you please let me know next steps for the PR?

adilGhaffarDev

/lgtm
Lets merge this and also the documentation.
@lentzi90 @peppi-lotta

peppi-lotta · 2025-04-23T07:05:30Z

/unhold

metal3-io-bot requested review from Sunnatillo and lentzi90 April 10, 2025 08:26

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 10, 2025

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 10, 2025

tuminoid added this to the IPAM - v1.11 milestone Apr 10, 2025

metal3-io-bot requested review from adilGhaffarDev, kashifest and peppi-lotta April 10, 2025 09:21

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2025

ksahil12 force-pushed the feature-allocate-IP branch from e6a046b to abd222a Compare April 11, 2025 12:29

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2025

lentzi90 reviewed Apr 17, 2025

View reviewed changes

ksahil12 force-pushed the feature-allocate-IP branch from abd222a to 1351201 Compare April 17, 2025 09:34

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2025

ksahil12 force-pushed the feature-allocate-IP branch 2 times, most recently from 1973d2b to 73edb0f Compare April 17, 2025 09:48

lentzi90 reviewed Apr 17, 2025

View reviewed changes

ipam/ippool_manager_test.go Outdated Show resolved Hide resolved

ipam/ippool_manager_test.go Outdated Show resolved Hide resolved

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2025

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2025

Adding feature for allocating requested IP

565bc08

Signed-off-by: khandelwalsahil <[email protected]>

ksahil12 force-pushed the feature-allocate-IP branch from 73edb0f to 565bc08 Compare April 17, 2025 09:55

Rozzii added this to Metal3 - Roadmap Apr 17, 2025

Rozzii moved this to IPAM WIP in Metal3 - Roadmap Apr 17, 2025

adilGhaffarDev reviewed Apr 23, 2025

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2025

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2025

metal3-io-bot merged commit c8a6082 into metal3-io:main Apr 23, 2025
13 checks passed

metal3-io-bot modified the milestones: IPAM - v1.11, IPAM - v1.10 Apr 23, 2025

lentzi90 moved this from IPAM WIP to Done / Closed in Metal3 - Roadmap Aug 21, 2025

✨ Adding feature for allocating requested IP #997

✨ Adding feature for allocating requested IP #997

Uh oh!

Conversation

ksahil12 commented Apr 10, 2025

Uh oh!

metal3-io-bot commented Apr 10, 2025

Uh oh!

tuminoid commented Apr 10, 2025

Uh oh!

peppi-lotta commented Apr 11, 2025

Uh oh!

peppi-lotta commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksahil12 commented Apr 11, 2025

Uh oh!

peppi-lotta commented Apr 11, 2025

Uh oh!

ksahil12 commented Apr 12, 2025

Uh oh!

peppi-lotta commented Apr 14, 2025

Uh oh!

peppi-lotta commented Apr 15, 2025

Uh oh!

ksahil12 commented Apr 15, 2025

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

ksahil12 commented Apr 17, 2025

Uh oh!

peppi-lotta commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

metal3-io-bot commented Apr 17, 2025

Uh oh!

ksahil12 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lentzi90 commented Apr 17, 2025

Uh oh!

ksahil12 commented Apr 17, 2025

Uh oh!

lentzi90 commented Apr 17, 2025

Uh oh!

lentzi90 commented Apr 17, 2025

Uh oh!

adilGhaffarDev commented Apr 17, 2025

Uh oh!

lentzi90 commented Apr 17, 2025

Uh oh!

ksahil12 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksahil12 commented Apr 22, 2025

Uh oh!

adilGhaffarDev left a comment

Choose a reason for hiding this comment

Uh oh!

peppi-lotta commented Apr 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

peppi-lotta commented Apr 11, 2025 •

edited

Loading

peppi-lotta commented Apr 17, 2025 •

edited

Loading

ksahil12 commented Apr 17, 2025 •

edited

Loading

ksahil12 commented Apr 17, 2025 •

edited

Loading